ref(explore): add some missing default renderers to getFieldRenderer #95231
Conversation
github-actions
bot
added
the
Scope: Frontend
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #95231 +/- ##
==========================================
+ Coverage 84.18% 85.34% +1.15%
==========================================
Files 10469 10448 -21
Lines 605430 603571 -1859
Branches 23677 23517 -160
==========================================
+ Hits 509687 515095 +5408
+ Misses 95222 88112 -7110
+ Partials 521 364 -157 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -352,3 +363,23 @@ function filterSeriesSortOptions(columns: Set<string>) { | |||
| return columns.has(option.value.meta.name); | |||
| }; | |||
| } | |||
|
|
|||
| function renderEventInTraceView( | |||
| data: any, | |||
There was a problem hiding this comment.
mb i was copying it from errorsAndTransactions.tsx... probably should fix it there too LOL
There was a problem hiding this comment.
On the topic of refactoring, a lot of tables reuse the same pattern for 'id' and 'trace' but I think the main difference is Source type changing. If we ever get to refactoring baggage, I wonder if we can make it flow through getFieldRender instead? I can add a note to that ticket that was made
I didn't do it in this PR because it would make it a lot more complicated 🥲
There was a problem hiding this comment.
Yeah, I think better baggage is a good idea! I want to copy the idea the Explore engineers used on the attributes tree, which is passing an object called renderExtras. I want to avoid cases where you have to pass span.description, span.op and span.module alongside the data, or it won't render. Better to put that stuff in renderExtras, and if it's there, rich rendering is enabled, for example
| @@ -143,6 +152,9 @@ const missingUserMisery = tct( | |||
| ), | |||
| } | |||
| ); | |||
| const userAgentLocking = t( | |||
| 'This OS locks newer versions to this version in the user-agent HTTP header. The exact OS version is unknown.' | |||
There was a problem hiding this comment.
| 'This OS locks newer versions to this version in the user-agent HTTP header. The exact OS version is unknown.' | |
| 'This operating system does not provide detailed version information in the User-Agent HTTP header. The exact operating system version is unknown.' |
| @@ -517,7 +499,33 @@ const SPECIAL_FIELDS: SpecialFields = { | |||
| sortField: 'span.description', | |||
| renderFunc: data => { | |||
| const value = data['span.description']; | |||
|
|
|||
| const op: string = data['span.op']; | |||
There was a problem hiding this comment.
| const op: string = data['span.op']; | |
| const op: string = data[SpanFields.SPAN_OP]; |
Small nit, but I'll need to track these down soon, so this'd help!
| typeof data.project_id === 'number' | ||
| ? data.project_id | ||
| : parseInt(data.project_id, 10) || -1; | ||
| const spanGroup: string | undefined = data['span.group']; |
There was a problem hiding this comment.
| const spanGroup: string | undefined = data['span.group']; | |
| const spanGroup: string | undefined = data[SpanFields.SPAN_GROUP]; |
| if (op === ModuleName.DB) { | ||
| return ( | ||
| <SpanDescriptionCell | ||
| description={value} | ||
| moduleName={op} | ||
| projectId={projectId} | ||
| group={spanGroup} | ||
| /> | ||
| ); | ||
| } | ||
| if (op === ModuleName.RESOURCE) { | ||
| return ( | ||
| <SpanDescriptionCell | ||
| description={value} | ||
| moduleName={op} | ||
| projectId={projectId} | ||
| group={spanGroup} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
| if (op === ModuleName.DB) { | |
| return ( | |
| <SpanDescriptionCell | |
| description={value} | |
| moduleName={op} | |
| projectId={projectId} | |
| group={spanGroup} | |
| /> | |
| ); | |
| } | |
| if (op === ModuleName.RESOURCE) { | |
| return ( | |
| <SpanDescriptionCell | |
| description={value} | |
| moduleName={op} | |
| projectId={projectId} | |
| group={spanGroup} | |
| /> | |
| ); | |
| } | |
| if ([ModuleName.DB, ModuleName.RESOURCE].includes(op)) { | |
| return ( | |
| <SpanDescriptionCell | |
| description={value} | |
| moduleName={op} | |
| projectId={projectId} | |
| group={spanGroup} | |
| /> | |
| ); | |
| } |
| @@ -649,6 +668,31 @@ const SPECIAL_FIELDS: SpecialFields = { | |||
| ); | |||
| }, | |||
| }, | |||
| project_id: { | |||
There was a problem hiding this comment.
Do you know offhand what the difference between project_id and project.id is? I'm wondering if one or the other is deprecated, and what the correct one is
There was a problem hiding this comment.
Hmm, I guess errors and transactions tables only have project_id whereas spans have both. If project_id is being deprecated later, we could just add it for both and remove later?
There was a problem hiding this comment.
Yes please! Both for now, and if you could dd a comment to explain, that'd be good
| sortField: 'project_id', | ||
| renderFunc: (data, {organization}) => { | ||
| const projectId = data.project_id; | ||
| // TODO: add projects to baggage to avoid using deprecated component |
There was a problem hiding this comment.
Could you explain this a bit more? What component should this be using?
There was a problem hiding this comment.
There is no component to use right now, the replacement is to use the useProjects hook. I guess its a bigger question of how this will get refactored
There was a problem hiding this comment.
Ah gotcha, in that case nevermind
| timestamp: { | ||
| sortField: 'timestamp', | ||
| renderFunc: data => { | ||
| const timestamp = data.timestamp; | ||
| if (!timestamp) { | ||
| return <Container>{emptyStringValue}</Container>; | ||
| } | ||
| const date = new Date(data.timestamp); | ||
| return ( | ||
| <Container> | ||
| <TimeSince unitStyle="extraShort" date={date} tooltipShowSeconds /> | ||
| </Container> | ||
| ); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🤔 I don't think this works as the default renderer for timestamps, timestamps should by default be rendered as a full date string (like in the spec). Which UIs use relative dates for the timestamp field?
There was a problem hiding this comment.
Traces table in explore, i can revert it back if it makes more sense to have it as the date string 👍
There was a problem hiding this comment.
Yeah, better as date string here. I think for Explore we should do something slightly different, like have a field called relative_time that will explicitly be different
| const broswerArray = browser.split(' '); | ||
| broswerArray.pop(); |
There was a problem hiding this comment.
Could you add a little comment to explain what's getting removed from the browser field here?
There was a problem hiding this comment.
Also, should be "browser" not "broswer"
| return <Container>{emptyStringValue}</Container>; | ||
| } | ||
|
|
||
| const formattedName = osName.split(' ').join('-').toLocaleLowerCase(); |
There was a problem hiding this comment.
Looks like this happens a bunch, maybe good to add a util like getContextIconName?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Bug: Malformed Project URLs with Trailing Slash
The getProjectIdLink function generates malformed project URLs. The path in makeProjectsPathname includes an incorrect trailing slash after the query parameter (/${project?.slug}/?project=${parsedId}/), leading to invalid URLs like /project-name/?project=123/. The correct format should be /${project?.slug}/?project=${parsedId}.
static/app/utils/discover/fieldRenderers.tsx#L997-L1000
sentry/static/app/utils/discover/fieldRenderers.tsx
Lines 997 to 1000 in 2b38268
Was this report helpful? Give feedback by reacting with 👍 or 👎
| @@ -1184,7 +1184,7 @@ describe('EventView.generateQueryStringObject()', function () { | |||
| name: 'best query', | |||
| fields: [ | |||
| {field: 'count()', width: 123}, | |||
| {field: 'project.id', width: 456}, | |||
There was a problem hiding this comment.
project.id should be sortable now? at least when i explicitly set its sortField as null, a newer test broke because it was expecting it to be sorted 🤔. I replaced it with issue since that isn't sortable.
Changes
There exists some cell renderers that exist throughout Sentry that should be used as defaults in the general renderer. This PR tries to add as many of them.
For a clear list of renderers added: refer to https://linear.app/getsentry/issue/DAIN-690/add-more-standard-renderers-for-event-data. I also have a dashboard that showcases these renderers:
dashboard/138017/